ROX-33198: Instrument inode tracking on file open lsm hook#391
ROX-33198: Instrument inode tracking on file open lsm hook#391JoukoVirtanen wants to merge 3 commits intomainfrom
Conversation
fact-ebpf/src/bpf/main.c
Outdated
| // to avoid verifier issues with untrusted pointers. | ||
| // We need to replicate the logic from inode_to_key() to handle | ||
| // special filesystems like btrfs correctly. | ||
| inode_key_t parent_key = {0}; |
There was a problem hiding this comment.
I tried to get the key for the parent using inode_to_key, but got a verifier error. That function is largely copied here. It is not completely copied, because doing so resulted in a verifier error. This will not work in all cases.
There was a problem hiding this comment.
What is the verifier error and or the code you were using that got the error? We really shouldn't duplicate code and I'm sure we can figure out a way to make the verifier happy.
There was a problem hiding this comment.
I am now using inode_to_key here without getting a verifier error. I modified inode_to_key so that it is able take in an untrusted pointer.
| return Ok(()); | ||
| } | ||
|
|
||
| let host_path = host_info::prepend_host_mount(event.get_filename()); |
There was a problem hiding this comment.
The initial plan was to look up the parent, get its file path, and then add the file name. However, it seems possible to get the entire path from the event.
There was a problem hiding this comment.
This only works if the file is not mounted to a different directory. Try monitoring /etc/monitored on your host, then run something like docker run --rm -it -v /etc/monitored:/in-container fedora:43 and access some files in /in-container. You should see the events have a filename in /in-container with a host path of /etc/monitored.
| # Wait for creation event | ||
| process = Process.from_proc() | ||
| creation_event = Event(process=process, event_type=EventType.CREATION, | ||
| file=fut, host_path='') |
There was a problem hiding this comment.
Should host_path be populated here.
| """ | ||
| cwd = os.getcwd() | ||
| config = { | ||
| 'paths': [f'{monitored_dir}/**', '/mounted/**', '/container-dir/**'], |
There was a problem hiding this comment.
I am not sure what globbing to use here.
fact-ebpf/src/bpf/main.c
Outdated
| // to avoid verifier issues with untrusted pointers. | ||
| // We need to replicate the logic from inode_to_key() to handle | ||
| // special filesystems like btrfs correctly. | ||
| inode_key_t parent_key = {0}; |
There was a problem hiding this comment.
What is the verifier error and or the code you were using that got the error? We really shouldn't duplicate code and I'm sure we can figure out a way to make the verifier happy.
| if path.is_file() || path.is_dir() { | ||
| self.metrics.scan_inc(ScanLabels::FileScanned); |
There was a problem hiding this comment.
Would be nice to have separate metrics here, so we can check how many files and directories we are tracking exactly.
| return Ok(()); | ||
| } | ||
|
|
||
| let host_path = host_info::prepend_host_mount(event.get_filename()); |
There was a problem hiding this comment.
This only works if the file is not mounted to a different directory. Try monitoring /etc/monitored on your host, then run something like docker run --rm -it -v /etc/monitored:/in-container fedora:43 and access some files in /in-container. You should see the events have a filename in /in-container with a host path of /etc/monitored.
| .with_context(|| format!("Failed to add creation event entry for {}", host_path.display()))?; | ||
| } else { | ||
| debug!("Creation event for non-existent file: {}", host_path.display()); | ||
| self.metrics.scan_inc(ScanLabels::FileRemoved); |
There was a problem hiding this comment.
It's probably worth it to add a separate label for this case, missing a host file is a separate condition IMO.
| if event.is_creation() { | ||
| if let Err(e) = self.handle_creation_event(&event) { |
There was a problem hiding this comment.
If we change edition in fact/Cargo.toml to 2024 we can rewrite this like:
if event.is_creation() &&
let Err(e) = self.handle_creation_event(&event) {|
|
||
| /// Handle file creation events by adding new inodes to the map. | ||
| fn handle_creation_event(&self, event: &Event) -> anyhow::Result<()> { | ||
| if self.get_host_path(Some(event.get_inode())).is_some() { |
There was a problem hiding this comment.
This condition can never be true, the events coming from the kernel don't have the host path, I would remove the block.
There was a problem hiding this comment.
Why do we need a separate test for this? I assume you should be able to tweak the tests in test_file_open.py to test your changes.
… can now use untrusted pointers
| } | ||
|
|
||
| unsigned long magic = inode->i_sb->s_magic; | ||
| unsigned long magic = BPF_CORE_READ(inode, i_sb, s_magic); |
There was a problem hiding this comment.
inode_to_key needs to be able to use untrusted pointers, so that it can get the key for a parent inode. To get it to be able to use untrusted pointers, BPF_CORE_READ is used.
2 participants
Description
A detailed explanation of the changes in your PR.
Feel free to remove this section if it is overkill for your PR, and the title of your PR is sufficiently descriptive.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why you did not do so. (Valid reasons include "CI is sufficient" or "No testable changes")
In addition to reviewing your code, reviewers must also review your testing instructions, and make sure they are sufficient.
For more details, ref the Confluence page about this section.